honor app name in arguments - #395
Conversation
b-admike
left a comment
b-admike
left a comment
There was a problem hiding this comment.
Do we have a test for when validation fails for the required name? Otherwise, LGTM.
| this.name = this.arguments[0]; | ||
| var valid = validateRequiredName(this.name); | ||
| if (valid === true) return; | ||
| else { |
There was a problem hiding this comment.
nitpick: can we have the curly brackets for the if condition above this else statement?
There was a problem hiding this comment.
No need to use else after return.
if (valid === true) return;
helpers.reportValidationError(valid, this.log);
this.name = undefined;
| base: 'PersistedModel', | ||
| }).then(function() { | ||
| var product = readProductJsonSync(); | ||
| expect(product).to.have.property('base', 'PersistedModel'); |
There was a problem hiding this comment.
I find this slightly confusing - how is the base property related to the model name provided via CLI arguments? Should you perhaps check that name property was set to Product instead?
There was a problem hiding this comment.
@bajtos if you look at the tests above, all the model cli tests create a model called Product, and check the result by asserting the model config is re-generated.
The test before it sets the base to CustomModel, therefore my test reset it to PersistedModel and assert accordingly.
Should you perhaps check that name property was set to Product instead?
I could add expect(product).to.have.property('name', 'Product');, but even the test fail this assertion would still be true :)
There was a problem hiding this comment.
The test before it sets the base to CustomModel, therefore my test reset it to PersistedModel and assert accordingly.
Could you please add a comment to explain this?
In general, tests should be independent on each other and don't rely on side effects.
but even the test fail this assertion would still be true :)
Isn't the same true for your current test in case only this single test is executed and all other tests are skipped? Either via it.only or in CLI via mocha -g "loopback:model generator.*honors the model name in arg".
I think you should explicitly set a different base model in this test, one that's not the default and also not used by any other tests around. Perhaps Model may work? Then verify that this explicit value was written to the file.
There was a problem hiding this comment.
@bajtos Hmm I see your concern. I rewrote the test, hopefully it makes more sense.
|
@b-admike Sorry i missed your comment, we usually don't test the printed warning. And since the I manually verified it: The invalid name warning is printed and the default name is reset to undefined. But good catch though :) turns out the helper function doesn't really print it, I switch to |
94962bf to
a4d8b08
Compare
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
Much better 👏
Please clean up the git commit history before landing.
a4d8b08 to
e2d772a
Compare
3 participants
Description
Fixing the test failure in strongloop/loopback-cli#71, it's also a missing feature.
Related issues
strongloop/loopback-cli#76
guide